fix(a11y): make artwork lightbox triggers and interactive rows keyboard-accessible#69
Open
Copilot wants to merge 5 commits into
Open
fix(a11y): make artwork lightbox triggers and interactive rows keyboard-accessible#69Copilot wants to merge 5 commits into
Copilot wants to merge 5 commits into
Conversation
Copilot
AI
changed the title
[WIP] Fix accessibility for artwork lightbox triggers
fix(a11y): make artwork lightbox triggers and interactive rows keyboard-accessible
May 19, 2026
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 11 medium 5 high |
🟢 Metrics 6 complexity · -2 duplication
Metric Results Complexity 6 Duplication -2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
InstaZDLL
force-pushed
the
copilot/fix-accessibility-lightbox-triggers
branch
from
c341b8e to
fdee4d5
Compare
github-actions
Bot
added
scope: frontend
InstaZDLL
force-pushed
the
copilot/fix-accessibility-lightbox-triggers
branch
from
64fb2ba to
04c6e0a
Compare
… keyboard-accessible
Follow-up to Copilot's a11y pass on this branch. Codacy raised two
classes of findings:
1. Ten MEDIUM "JSX attribute is specific to React" — PMD running on
`.tsx`/`.jsx` and flagging `className` as if it were JSP. PMD
targets Java / Apex / PLSQL / JSP — none of which exist in this
repo. Drop PMD (and the Java runtime it pulled in) from
`.codacy/codacy.yaml`; eslint already covers JS/TS, Opengrep +
Trivy cover security, Lizard covers complexity.
2. Nine HIGH `jsx-a11y/*` findings — `<li>` / `<div>` non-interactive
elements carrying `tabIndex` + `role="button"`. Split the fix two
ways:
- Lyrics rows (`LyricsPanel`, `FullscreenLyrics`) and the now-
playing `QueueRow` (interactive variant was dead code anyway):
refactor into real `<button>` elements. `onKeyDown` becomes
unnecessary — `<button>` natively handles Enter and Space. Net
code reduction.
- `SortableQueueRow` and the six `TrackTable` rows: contain inner
`<button>` elements (drag handle, like, more-options), so the
outer can't be a `<button>` without producing nested-button
invalid HTML. Keep the `<div tabIndex role="button">` pattern
with bare `// eslint-disable-next-line` + comment explaining
why the compound is intentional; keyboard activation still
works end-to-end.
The bare disable directives target jsx-a11y rules that Codacy runs
server-side but that we don't load locally (no
`eslint-plugin-jsx-a11y` dep). Set linterOptions.reportUnusedDisableDirectives
to "off" in `eslint.config.js` so eslint v9 doesn't flag them as
unused locally.
Codacy runs its a11y checks via a cloud-side analyzer that doesn't read the repo's eslint config and doesn't honor inline `eslint-disable` directives. The seven bare `// eslint-disable-next-line` lines added in the previous commit were therefore load-bearing for nothing and only produced "Unused eslint-disable directive" warnings locally. Drop them along with the matching `reportUnusedDisableDirectives: "off"` opt-out in `eslint.config.js`. The explanatory comments above each compound row stay — they still document why the `<div tabIndex role="button">` pattern is intentional for rows that contain inner buttons.
The Codacy CLI install script leaked into the previous commit despite being declared in `.codacy/.gitignore` — that file is itself untracked, so its rules only apply on machines that already have it. Move the exclusion rules to the tracked root `.gitignore` so a fresh checkout doesn't accumulate the same machine-local artefacts.
InstaZDLL
force-pushed
the
copilot/fix-accessibility-lightbox-triggers
branch
from
04c6e0a to
a988812
Compare
InstaZDLL
approved these changes
May 20, 2026
Code Review by Qodo
1. Bubbled keydown plays track
|
InstaZDLL
force-pushed
the
copilot/fix-accessibility-lightbox-triggers
branch
from
b3ad445 to
a988812
Compare
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1106
to
+1114
| tabIndex={0} | ||
| onClick={(e) => onRowSelect(track, e)} | ||
| onDoubleClick={() => onPlayTrack(index)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| onPlayTrack(index); | ||
| } | ||
| }} |
There was a problem hiding this comment.
1. Bubbled keydown plays track 🐞 Bug ≡ Correctness
Multiple track-table rows (including SortableQueueRow) attach an Enter/Space onKeyDown that plays/jumps to a track but don’t ensure the row itself is the key event target, so keyboard activation on nested interactive elements (e.g., like/menu/ArtistLink/AlbumLink buttons and the drag-handle button) can bubble up and trigger unintended playback/jumps. This breaks keyboard interaction by causing unexpected track changes while operating in-row controls.
Agent Prompt
## Issue description
Several track-table row containers (including `SortableQueueRow`) handle `Enter`/`Space` in `onKeyDown` to play/jump to the row’s track, but the handler can run even when the key event originated from a nested interactive element (e.g., like button, add-to-playlist/menu button, ArtistLink/AlbumLink buttons, or the queue drag-handle `<button>`). Because nested controls typically only stop propagation for mouse events, keyboard `keydown` can still bubble to the row and trigger unintended playback/jumps.
## Issue Context
These rows intentionally contain nested interactive controls (buttons/links), and the queue drag-handle already stops propagation for `click`/`doubleClick` but not for keyboard events. The row-level Enter/Space behavior should only trigger when the row itself is the focused/targeted element (or nested controls should explicitly prevent keydown propagation).
## Fix Focus Areas
- src/components/views/LibraryView.tsx[1104-1114]
- src/components/views/PlaylistView.tsx[1107-1118]
- src/components/views/LikedView.tsx[154-165]
- src/components/views/AlbumDetailView.tsx[472-482]
- src/components/views/ArtistDetailView.tsx[584-593]
- src/components/views/GenreDetailView.tsx[282-291]
- src/components/layout/QueuePanel.tsx[531-551]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo follow-up on the a11y branch. Track-row + queue-row containers handle Enter/Space to play / jump, but the handler also ran when the key event originated from a nested control (like button, `+` button, ArtistLink, AlbumLink, the drag-handle in QueuePanel). Nested buttons stop propagation for clicks but not for keydown, so pressing Enter on the like button also triggered the row's playback handler. Gate every row handler on `e.target === e.currentTarget` so the behaviour only fires when the row itself is the focused element. Covered: LibraryView, PlaylistView, LikedView, AlbumDetailView, ArtistDetailView, GenreDetailView, QueuePanel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NowPlayingPanel,AlbumDetailView,ArtistDetailView) were bare<div onClick>/<img onClick>— not focusable, not announced by screen readers, and broken foruseModalA11yfocus restoration (capturedactiveElementwas<body>, not the trigger).cursor-pointerpatterns found the same gap on lyric-seek rows, queue rows, WrappedView artist rows, and all six track-table views.Artwork triggers (
NowPlayingPanel,AlbumDetailView,ArtistDetailView):<button type="button">+aria-label={t("common.viewArtwork")}+focus-visible:ring-2 focus-visible:ring-emerald-500ArtistDetailView:pointer-events-noneoverlay +pointer-events-autoedit-pencil preserved; pencil is a sibling of the new button, not nested inside it, so clicks don't cross-fireuseModalA11yround-trip now works:document.activeElementis the<button>on open → focus returns to it on closeLyrics seek rows (
LyricsPanel,FullscreenLyrics):role="button",tabIndex={0},onKeyDown(Enter/Space → seek) to synced<li>elementsWrappedView top-artists:
<li onClick>→<li><button>, matching the already-accessible top-albums pattern in the same componentQueuePanel:
QueueRow+SortableQueueItem:tabIndex,role="button",onKeyDown(Enter/Space → jump to track)Track rows (
LibraryView,PlaylistView,LikedView,AlbumDetailView,ArtistDetailView,GenreDetailView):tabIndex={0}+onKeyDown(Enter/Space → play); focus ring usesfocus-visible:ring-inset focus-visible:ring-emerald-500i18n:
common.viewArtworkadded to all 17 locale files.How I tested
bun run lintandbun run typecheckboth pass cleanlyArtistDetailViewstill opens the image picker independently of the lightbox buttonScreenshots / clips
Checklist
type(scope): subject, kebab-case scope)bun run lint+bun run typecheckpass locallycargo check --manifest-path src-tauri/Cargo.toml --all-targetspasses locallysrc/i18n/locales/updatedCLAUDE.mdanddocs/updated